Merged
Conversation
angeloskath
force-pushed
the
use-after-move
branch
from
09b3708 to
ab0e1ef
Compare
Member
Author
|
Oh I see it was already fixed on main sorry for the noise. Well I 'll leave the PR as it does one less copy on that bug and also fixes the other asymptomatic ones. |
Collaborator
|
With SmallVector there is actually not much meaning to move shapes and strides since they no longer allocate on heap (unless it has very large ndim which is forbidden in CUDA backend), so std::move would just add overhead erasing the old object's memory to 0s. |
Member
Author
|
Interesting... force of habit I guess. Do you think we should remove the moves of Shape and Strides throughout ? There is probably more than 100 places. |
Collaborator
|
I think it does not worth the effort changing all existing code as it is not going to make much difference in performance, we should just avoid introducing new ones. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Well there is 1 very real bug and then the rest are officially bugs but don't really show up.
Basically I came across the
Broadcastuse after move now that I was looking into #3319 and I went to a small quest to find more. Well good news I didn't quite find many.The main pattern that is questionable is taking a reference to a shape and then moving the array. I think officially this is undefined behavior. The change here is actually incurring no overhead since it copies the shape but then moves it to the array constructor.